-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat: Implement SEP-986: Tool Name Guidance #1550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| @field_validator("name") | ||
| @classmethod | ||
| def _validate_tool_name(cls, value: str) -> str: | ||
| if not (1 <= len(value) <= 128): | ||
| raise ValueError(f"Invalid tool name length: {len(value)}. Tool name must be between 1 and 128 characters.") | ||
|
|
||
| if not TOOL_NAME_PATTERN.fullmatch(value): | ||
| raise ValueError("Invalid tool name characters. Allowed: A-Z, a-z, 0-9, underscore (_), dash (-), dot (.).") | ||
|
|
||
| return value | ||
|
|
||
| """ | ||
| See [MCP specification](https://modelcontextprotocol.io/specification/draft/server/tools#tool-names) | ||
| for more information on tool naming conventions. | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxisbey This is a less verbose way to write the same thing:
from typing import Annotated
from pydantic import StringConstraints
tool: Annotated[str, StringConstraints(max_length=128, pattern=r"^[A-Za-z0-9_.-]+$")]I'm not sure if this should be applied on BaseMetadata, but if not, please drop the inheritance, and apply this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, most or all the field_validators in this code source can be replaced by proper annotated versions - that you can see are cuter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't think there's a need to test that Pydantic is working as expected.
Implement SEP-986: Tool Name Guidance for Tool Type
Changes
re.compile(r"^[A-Za-z0-9_.-]+$")that raises an error for invalid characters:Motivation and Context
Related issues: SEP-986 #1537
Implements the guidance from modelcontextprotocol/modelcontextprotocol#986
How Has This Been Tested?
Unit Tests
Valid Values
getUserDATA_EXPORT_v2admin.tools.listaZ9_.-"x" * 128(max length)Invalid Values
""(empty string)"x" * 129(exceeds max length)has spacecomma,namenot/allowedname@name#name$Breaking Changes
Types of changes
Checklist
Additional context